Skip to content

feat: add verifiable catalog approval provenance - #519

Merged
imran-siddique merged 1 commit into
mainfrom
issue-517-approval-provenance
Aug 17, 2026
Merged

feat: add verifiable catalog approval provenance#519
imran-siddique merged 1 commit into
mainfrom
issue-517-approval-provenance

Conversation

@imran-siddique

Copy link
Copy Markdown
Member

SummarynnImplements #517 as a detached, signed catalog-change provenance record.nn- Adds versioned catalog approval schema and threat-boundary documentation.n- Uses Ed25519 signatures over the record and each approval; trust keys stay verifier-side.n- Enforces M-of-N approvals, distinct principals/roles, issuer and key binding, expiry, revocation, and chain checkpoints.n- Binds new_catalog_hash to the runtime catalog hash.n- Adds deterministic focused vectors for tampering, stale chain, revocation, expiry, and duplicate principals.nn## Evidencenn- Focused pytest: 4 passedn- Ruff: passedn- mypy approval module: passedn- JSON Schema meta-validation: passedn- git diff --check: passedn- Full local suite is environment-blocked at collection by pre-existing missing dependencies: agentrust_trace, agent_os, cedarpy, and mcp.n`nSigned-off-by: Imran Siddique imran@agentrust.com

Signed-off-by: Imran Siddique <imran.siddique@opaque.co>
@imran-siddique
imran-siddique merged commit ddfd9c3 into main Aug 17, 2026
13 checks passed
@imran-siddique
imran-siddique deleted the issue-517-approval-provenance branch August 17, 2026 16:35
zohebk8s pushed a commit to zohebk8s/cmcp that referenced this pull request Aug 20, 2026
The four tests that landed with agentrust-io#519 pass whether or not the policy, identity,
and validity checks exist. Deleting the threshold count, the distinct-role
guard, the role match, the principal and issuer match, or the interval order
check leaves the suite green, so the module's M-of-N claim rested on code
review alone.

Added:

* threshold shortfall, one approval against a 2-of-N policy
* repeated role under a distinct-role policy, with distinct principals so the
  principal guard cannot mask it
* principal, issuer, and role mismatches against the trusted key
* validity boundaries, approved_at inclusive, expires_at exclusive, and an
  inverted interval
* a genesis record at sequence 1, which fixes the all-zero previous_record_hash
  as the convention until the schema says otherwise
* a schema and verifier agreement test over eleven malformed records, asserting
  that whatever the shipped schema rejects the verifier rejects too

Each new test was checked by removing the guard it covers and confirming the
test fails. The agreement test is the one that would have caught the bool
timestamp divergence.

One case is marked xfail strict: the schema sets a minimum of zero on
approved_at and expires_at, the verifier does not, so a record with a negative
approval timestamp verifies. That is left failing on purpose, since fixing it
belongs with the decision on whether the verifier loads the schema at all.

The schema is read from the repository at tests/../schemas, following
test_trace_claim.py, because it is still absent from the wheel.

Signed-off-by: Mohammed Zoheb Shaik <zoheb.shaik7@gmail.com>
imran-siddique pushed a commit that referenced this pull request Aug 21, 2026
…n fields (#517) (#531)

* fix(catalog): pin the approval policy verifier-side and bind the chain fields

verify_catalog_change read threshold, distinct_principals, and distinct_roles
out of the record under verification, and checked policy_hash for digest shape
only. A single trusted reviewer key could therefore issue a record declaring a
threshold of one and have it verify, so the M-of-N property was unenforced.

expected_policy_hash and expected_catalog_id are now required keyword
arguments. The record's policy_hash must cover its own policy body and must
equal the policy the verifier was configured with. compute_policy_hash defines
that digest so producers and verifiers agree on it.

previous_catalog_hash, sequence, and catalog_id were format checked and then
unused. catalog_id is now always bound. expected_sequence and
expected_previous_catalog_hash join expected_previous_record_hash as optional
checkpoints, since those must come from an external pin.

Also in this pass:

* reject repeated principals and roles rather than counting distinct values,
  which admitted alice, bob, alice at a threshold of two
* validate string and integer field types, so a non-hashable role raises
  CatalogApprovalError instead of escaping as TypeError
* reject boolean timestamps, which the JSON Schema already rejected
* validate the signature alphabet and decoded length, and move the decode out
  of the try block where its errors were masked as "signature is invalid"
* drop the post-loop threshold checks, now unreachable

Not addressed here: approvals are still judged against wall clock, so a record
stops verifying once its approvals expire, and the JSON Schema is still not
loaded by the verifier or shipped in the wheel.

Signed-off-by: Mohammed Zoheb Shaik <zoheb.shaik7@gmail.com>

* test(catalog): cover the approval checks the suite never asserted

The four tests that landed with #519 pass whether or not the policy, identity,
and validity checks exist. Deleting the threshold count, the distinct-role
guard, the role match, the principal and issuer match, or the interval order
check leaves the suite green, so the module's M-of-N claim rested on code
review alone.

Added:

* threshold shortfall, one approval against a 2-of-N policy
* repeated role under a distinct-role policy, with distinct principals so the
  principal guard cannot mask it
* principal, issuer, and role mismatches against the trusted key
* validity boundaries, approved_at inclusive, expires_at exclusive, and an
  inverted interval
* a genesis record at sequence 1, which fixes the all-zero previous_record_hash
  as the convention until the schema says otherwise
* a schema and verifier agreement test over eleven malformed records, asserting
  that whatever the shipped schema rejects the verifier rejects too

Each new test was checked by removing the guard it covers and confirming the
test fails. The agreement test is the one that would have caught the bool
timestamp divergence.

One case is marked xfail strict: the schema sets a minimum of zero on
approved_at and expires_at, the verifier does not, so a record with a negative
approval timestamp verifies. That is left failing on purpose, since fixing it
belongs with the decision on whether the verifier loads the schema at all.

The schema is read from the repository at tests/../schemas, following
test_trace_claim.py, because it is still absent from the wheel.

Signed-off-by: Mohammed Zoheb Shaik <zoheb.shaik7@gmail.com>

* docs(changelog): record the catalog approval policy pinning fix

Signed-off-by: Mohammed Zoheb Shaik <zoheb.shaik7@gmail.com>

* test(catalog): point the xfail at #533

Signed-off-by: Mohammed Zoheb Shaik <zoheb.shaik7@gmail.com>

* test(catalog): cover the three branches the suite still missed

Codecov flagged one line of the diff: the policy distinctness flags guard, which
nothing exercised. Three more lines in the module were uncovered before this PR
and are cheap to reach from the same fixture, so they go with it.

* a policy flag set to a truthy string rather than a boolean
* an unknown profile
* a digest of the wrong shape as well as the wrong alphabet
* an approval carrying a stray member
* a one character signature, which is the only input that reaches the base64
  decode error path, since anything with a bad alphabet is rejected before it
  and anything longer decodes and fails on length

The module is now at 100 percent line coverage, 28 tests and the deliberate
xfail.

Signed-off-by: Mohammed Zoheb Shaik <zoheb.shaik7@gmail.com>

* test(catalog): carry the JCS round trip onto the new verifier signature

#532 landed first, and its end to end test calls verify_catalog_change, which
this branch gives two required keyword arguments. Left alone the test fails with
a TypeError on main once this merges, so the fix belongs here rather than in a
follow-up.

The record it builds is unchanged apart from computing policy_hash with
compute_policy_hash, which is what the verifier now requires the field to cover.
The point of the test is unaffected: a non-ASCII reviewer identity still signs
and verifies end to end.

Signed-off-by: Mohammed Zoheb Shaik <zoheb.shaik7@gmail.com>

* fix(catalog): count a reviewer key once, whatever the policy says

The M-of-N property still had a hole that the distinctness rules do not close,
because nothing bound approvals to distinct keys. Both of these verified before
this change:

* threshold 3 with distinct_principals false, one key signing the identical
  approval three times, accepted with valid_approvals 3
* threshold 2 with distinct_roles true, one key signing twice as "security" and
  "owner", accepted, because a TrustedReviewer with role None lets the record
  assert whatever role it likes

The first is arguably what the policy asked for, but a repeated signature is one
approval presented three times rather than three approvals. The second is not
what the policy asked for at all: it demands two roles and got one key.

A key now counts once per record, checked after the principal and role rules so
their errors keep reporting the more specific cause. Verified by removing the
guard: 2 of the 31 tests fail.

Signed-off-by: Mohammed Zoheb Shaik <zoheb.shaik7@gmail.com>

* docs(changelog): record the reviewer key reuse rule

Signed-off-by: Mohammed Zoheb Shaik <zoheb.shaik7@gmail.com>

* docs(spec): say that a key identifier must name one key

Signed-off-by: Mohammed Zoheb Shaik <zoheb.shaik7@gmail.com>

---------

Signed-off-by: Mohammed Zoheb Shaik <zoheb.shaik7@gmail.com>
Co-authored-by: Mohammed Zoheb Shaik <zoheb.shaik7@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants